-
Notifications
You must be signed in to change notification settings - Fork 43
fix: add MIME info change detection and reload #322
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Reviewer's GuideAdds automatic detection and reloading of MIME information when the user’s mimeapps.list changes, exposes a new DBus signal to notify clients, and protects against reload loops while updating related DBus and service plumbing. Sequence diagram for MIME info reload on mimeapps.list changessequenceDiagram
actor ExternalTool
participant FileSystem as QFileSystemWatcher
participant MimeManager as MimeManager1Service
participant DebounceTimer as QTimer_mimeAppsDebounce
participant AppManager as ApplicationManager1Service
participant Client as DBusClient
ExternalTool->>FileSystem: modify ~/.config/mimeapps.list
FileSystem-->>MimeManager: fileChanged(path)
alt path not in watcher.files() and file exists
MimeManager->>FileSystem: addPath(path)
end
alt internalWriteInProgress is true
MimeManager->>MimeManager: set internalWriteInProgress = false
Note over MimeManager: change ignored, no reload
else external change
MimeManager->>DebounceTimer: start(50ms)
end
DebounceTimer-->>MimeManager: timeout()
MimeManager->>MimeManager: handleMimeAppsFileDebounced()
MimeManager->>AppManager: reloadMimeInfos()
AppManager->>MimeManager: m_mimeManager->reset()
AppManager->>AppManager: scanMimeInfos()
AppManager-->>MimeManager: emit MimeInfoChanged
MimeManager-->>Client: DBus signal MimeInfoChanged
Updated class diagram for MIME manager reload and DBus signalclassDiagram
class ApplicationManager1Service {
+void initService(QDBusConnection &connection) noexcept
+void reloadMimeInfos() noexcept
+void scanMimeInfos() noexcept
+QSharedPointer~ApplicationService~ addApplication(DesktopFile desktopFileSource) noexcept
+void removeOneApplication(QString appId) noexcept
+void removeAllApplication() noexcept
}
class MimeManager1Service {
+MimeManager1Service(ApplicationManager1Service *parent)
+~MimeManager1Service()
+void setDefaultApplication(QStringMap defaultApps) noexcept
+void unsetDefaultApplication(QStringList mimeTypes) noexcept
+signal void MimeInfoChanged()
-void onMimeAppsFileChanged(QString path)
-void handleMimeAppsFileDebounced()
-QMimeDatabase m_database
-vector~MimeInfo~ m_infos
-QFileSystemWatcher m_mimeAppsWatcher
-bool m_internalWriteInProgress
-QTimer m_mimeAppsDebounceTimer
}
class MimeManager1Adaptor {
+MimeManager1Adaptor(MimeManager1Service *parent)
+public slots void setDefaultApplication(QStringMap defaultApps)
+public slots void unsetDefaultApplication(QStringList mimeTypes)
+signal void MimeInfoChanged()
}
class QFileSystemWatcher {
+void addPath(QString path)
+QStringList files() const
+signal void fileChanged(QString path)
}
class QTimer {
+void setSingleShot(bool singleShot)
+void setInterval(int msec)
+void start()
+signal void timeout()
}
ApplicationManager1Service --> MimeManager1Service : owns m_mimeManager
MimeManager1Service ..> ApplicationManager1Service : parent()
MimeManager1Service --> QFileSystemWatcher : uses m_mimeAppsWatcher
MimeManager1Service --> QTimer : uses m_mimeAppsDebounceTimer
MimeManager1Adaptor ..> MimeManager1Service : adapts to DBus
MimeManager1Adaptor ..> MimeManager1Service : forwards MimeInfoChanged signal
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey - I've found 3 issues, and left some high level feedback:
- The
m_internalWriteInProgressflag is only reset on write failure or when a file change event arrives, so ifwriteToFile()succeeds but the watcher never emitsfileChanged(e.g. no actual on-disk change), the flag may stay true and cause the next external change to be skipped; consider resetting it explicitly after a successful reload or with a timeout. - The watcher currently adds
mimeapps.listonly if it exists at construction time and onfileChanged, so if the file is created after startup it will never be watched; consider also watching the config directory (or re-checking periodically) so that a newly createdmimeapps.listis picked up.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `m_internalWriteInProgress` flag is only reset on write failure or when a file change event arrives, so if `writeToFile()` succeeds but the watcher never emits `fileChanged` (e.g. no actual on-disk change), the flag may stay true and cause the next external change to be skipped; consider resetting it explicitly after a successful reload or with a timeout.
- The watcher currently adds `mimeapps.list` only if it exists at construction time and on `fileChanged`, so if the file is created after startup it will never be watched; consider also watching the config directory (or re-checking periodically) so that a newly created `mimeapps.list` is picked up.
## Individual Comments
### Comment 1
<location> `src/dbus/applicationmanager1service.cpp:282` </location>
<code_context>
+ qInfo() << "Reloading MIME info due to external configuration change.";
+ m_mimeManager->reset();
+ scanMimeInfos();
+ emit m_mimeManager->MimeInfoChanged();
+}
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Emitting another object's signal directly from here likely violates C++ access control and may not compile
Because Q_SIGNALS makes MimeInfoChanged() a protected member, it can only be called from within MimeManager1Service or its friends/derived classes. ApplicationManager1Service cannot legally call m_mimeManager->MimeInfoChanged() directly. Instead, consider adding a public method on MimeManager1Service (e.g., notifyMimeInfoChanged()) that emits the signal internally, or emit a signal on ApplicationManager1Service and connect it to MimeManager1Service::MimeInfoChanged().
</issue_to_address>
### Comment 2
<location> `src/dbus/mimemanager1service.cpp:181-182` </location>
<code_context>
+
+void MimeManager1Service::onMimeAppsFileChanged(const QString &path)
+{
+ if (!m_mimeAppsWatcher.files().contains(path) && QFileInfo::exists(path)) {
+ m_mimeAppsWatcher.addPath(path);
+ }
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** File-based watching may miss changes when mimeapps.list is deleted and later recreated
QFileSystemWatcher will stop watching the path once mimeapps.list is removed, but your handler only re-adds it if QFileInfo::exists(path) is true at notification time. If the file is deleted and later recreated, you may never reattach the watcher and will miss subsequent changes. Consider also watching the parent directory and re-adding the file when it is (re)created, or otherwise handling the case where the file doesn’t exist at the moment of the change signal.
Suggested implementation:
```cpp
void MimeManager1Service::onMimeAppsFileChanged(const QString &path)
{
const QString userMimeAppsFile = getXDGConfigHome() + "/mimeapps.list";
// Ensure we (re)attach a file watcher whenever mimeapps.list exists but is not watched.
// QFileSystemWatcher stops watching a file once it is removed, so this covers the
// "delete + recreate" case when combined with a directory watcher on the parent dir.
if (!m_mimeAppsWatcher.files().contains(userMimeAppsFile) && QFileInfo::exists(userMimeAppsFile)) {
m_mimeAppsWatcher.addPath(userMimeAppsFile);
}
// Debounce downstream handling of mimeapps.list changes
m_mimeAppsDebounceTimer.setSingleShot(true);
if (!m_mimeAppsDebounceTimer.isActive()) {
m_mimeAppsDebounceTimer.start();
}
}
```
To fully implement the suggestion and cover the delete + recreate scenario, you should also:
1. **Watch the parent directory (likely in the MimeManager1Service constructor or initialization code):**
- Determine the XDG config directory (same `getXDGConfigHome()` you use for the file).
- Add it to the watcher if not already present:
```cpp
const QString userConfigDir = getXDGConfigHome();
if (!m_mimeAppsWatcher.directories().contains(userConfigDir) && QFileInfo::exists(userConfigDir)) {
m_mimeAppsWatcher.addPath(userConfigDir);
}
```
- Connect the `directoryChanged` signal to a new slot:
```cpp
connect(&m_mimeAppsWatcher, &QFileSystemWatcher::directoryChanged,
this, &MimeManager1Service::onMimeAppsDirectoryChanged);
```
2. **Implement the `onMimeAppsDirectoryChanged` slot to re-attach the file watcher when mimeapps.list is (re)created:**
```cpp
void MimeManager1Service::onMimeAppsDirectoryChanged(const QString &dirPath)
{
const QString userConfigDir = getXDGConfigHome();
if (dirPath != userConfigDir) {
return;
}
const QString userMimeAppsFile = userConfigDir + "/mimeapps.list";
// If the file now exists but isn't being watched, re-add it.
if (QFileInfo::exists(userMimeAppsFile) &&
!m_mimeAppsWatcher.files().contains(userMimeAppsFile)) {
m_mimeAppsWatcher.addPath(userMimeAppsFile);
// Optionally treat this as a "file changed" event so existing logic runs:
onMimeAppsFileChanged(userMimeAppsFile);
}
}
```
3. **Ensure the declaration of `onMimeAppsDirectoryChanged` is added to the corresponding header (`mimemanager1service.h`), e.g.:**
```cpp
private slots:
void onMimeAppsFileChanged(const QString &path);
void onMimeAppsDirectoryChanged(const QString &dirPath);
```
With these changes, the parent directory is always watched, and the file watcher is automatically reattached when `mimeapps.list` is deleted and later recreated.
</issue_to_address>
### Comment 3
<location> `src/dbus/mimemanager1service.cpp:28` </location>
<code_context>
+ m_mimeAppsWatcher.addPath(userMimeAppsFile);
+ }
+
+ m_mimeAppsDebounceTimer.setSingleShot(true);
+ m_mimeAppsDebounceTimer.setInterval(50);
+ connect(&m_mimeAppsDebounceTimer, &QTimer::timeout, this, &MimeManager1Service::handleMimeAppsFileDebounced);
</code_context>
<issue_to_address>
**issue (complexity):** Consider replacing the shared `m_internalWriteInProgress` flag with scoped `QSignalBlocker` usage and an optional helper for debounce to localize state and simplify control flow.
You can simplify the control flow and remove the fragile `m_internalWriteInProgress` state entirely by scoping the suppression of `fileChanged` using `QSignalBlocker` on the watcher. That keeps the “ignore internal writes” behaviour but makes it local and non‑leaky.
### 1. Replace `m_internalWriteInProgress` with a scoped signal blocker
```cpp
#include <QSignalBlocker>
```
```cpp
void MimeManager1Service::setDefaultApplication(const ObjectMap &defaultApps) noexcept
{
auto &app = m_infos.front().appsList();
auto userConfig = std::find_if(app.begin(), app.end(), [](const MimeApps &config) {
return !config.isDesktopSpecific();
});
if (userConfig == app.end()) {
qWarning() << "couldn't find user mimeApps";
safe_sendErrorReply(QDBusError::InternalError);
return;
}
// Scoped suppression of QFileSystemWatcher notifications
QSignalBlocker watcherBlocker(&m_mimeAppsWatcher);
for (auto it = defaultApps.constKeyValueBegin(); it != defaultApps.constKeyValueEnd(); ++it) {
userConfig->setDefaultApplication(it->first, it->second);
}
if (!userConfig->writeToFile()) {
safe_sendErrorReply(QDBusError::Failed,
"set default app failed, these config will be reset after re-login.");
// no extra state to reset; watcherBlocker destructor re‑enables signals
return;
}
}
```
```cpp
void MimeManager1Service::unsetDefaultApplication(const QStringList &mimeTypes) noexcept
{
auto &app = m_infos.front().appsList();
auto userConfig = std::find_if(app.begin(), app.end(), [](const MimeApps &config) {
return !config.isDesktopSpecific();
});
if (userConfig == app.end()) {
qWarning() << "couldn't find user mimeApps";
safe_sendErrorReply(QDBusError::InternalError);
return;
}
QSignalBlocker watcherBlocker(&m_mimeAppsWatcher);
for (const auto &mime : mimeTypes) {
userConfig->unsetDefaultApplication(mime);
}
if (!userConfig->writeToFile()) {
safe_sendErrorReply(QDBusError::Failed,
"unset default app failed, these config will be reset after re-login.");
return;
}
}
```
Then `onMimeAppsFileChanged` no longer needs to know about internal write state:
```cpp
void MimeManager1Service::onMimeAppsFileChanged(const QString &path)
{
if (!m_mimeAppsWatcher.files().contains(path) && QFileInfo::exists(path)) {
m_mimeAppsWatcher.addPath(path);
}
m_mimeAppsDebounceTimer.start();
}
```
This:
- Keeps the feature (watching and reloading `mimeapps.list`).
- Ensures internal writes never trigger reloads.
- Removes the cross‑method lifecycle of `m_internalWriteInProgress` and its hidden coupling.
### 2. Optional: isolate debounce logic into a helper
To reduce the cognitive load further, you can hide the timer in a small helper and keep `onMimeAppsFileChanged` high‑level:
```cpp
void MimeManager1Service::scheduleMimeAppsReload()
{
m_mimeAppsDebounceTimer.start();
}
void MimeManager1Service::onMimeAppsFileChanged(const QString &path)
{
if (!m_mimeAppsWatcher.files().contains(path) && QFileInfo::exists(path)) {
m_mimeAppsWatcher.addPath(path);
}
scheduleMimeAppsReload();
}
```
This keeps all existing behaviour but makes the state management explicit, scoped, and easier to reason about.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| qInfo() << "Reloading MIME info due to external configuration change."; | ||
| m_mimeManager->reset(); | ||
| scanMimeInfos(); | ||
| emit m_mimeManager->MimeInfoChanged(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (bug_risk): Emitting another object's signal directly from here likely violates C++ access control and may not compile
Because Q_SIGNALS makes MimeInfoChanged() a protected member, it can only be called from within MimeManager1Service or its friends/derived classes. ApplicationManager1Service cannot legally call m_mimeManager->MimeInfoChanged() directly. Instead, consider adding a public method on MimeManager1Service (e.g., notifyMimeInfoChanged()) that emits the signal internally, or emit a signal on ApplicationManager1Service and connect it to MimeManager1Service::MimeInfoChanged().
| if (!m_mimeAppsWatcher.files().contains(path) && QFileInfo::exists(path)) { | ||
| m_mimeAppsWatcher.addPath(path); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (bug_risk): File-based watching may miss changes when mimeapps.list is deleted and later recreated
QFileSystemWatcher will stop watching the path once mimeapps.list is removed, but your handler only re-adds it if QFileInfo::exists(path) is true at notification time. If the file is deleted and later recreated, you may never reattach the watcher and will miss subsequent changes. Consider also watching the parent directory and re-adding the file when it is (re)created, or otherwise handling the case where the file doesn’t exist at the moment of the change signal.
Suggested implementation:
void MimeManager1Service::onMimeAppsFileChanged(const QString &path)
{
const QString userMimeAppsFile = getXDGConfigHome() + "/mimeapps.list";
// Ensure we (re)attach a file watcher whenever mimeapps.list exists but is not watched.
// QFileSystemWatcher stops watching a file once it is removed, so this covers the
// "delete + recreate" case when combined with a directory watcher on the parent dir.
if (!m_mimeAppsWatcher.files().contains(userMimeAppsFile) && QFileInfo::exists(userMimeAppsFile)) {
m_mimeAppsWatcher.addPath(userMimeAppsFile);
}
// Debounce downstream handling of mimeapps.list changes
m_mimeAppsDebounceTimer.setSingleShot(true);
if (!m_mimeAppsDebounceTimer.isActive()) {
m_mimeAppsDebounceTimer.start();
}
}
To fully implement the suggestion and cover the delete + recreate scenario, you should also:
-
Watch the parent directory (likely in the MimeManager1Service constructor or initialization code):
- Determine the XDG config directory (same
getXDGConfigHome()you use for the file). - Add it to the watcher if not already present:
const QString userConfigDir = getXDGConfigHome(); if (!m_mimeAppsWatcher.directories().contains(userConfigDir) && QFileInfo::exists(userConfigDir)) { m_mimeAppsWatcher.addPath(userConfigDir); }
- Connect the
directoryChangedsignal to a new slot:connect(&m_mimeAppsWatcher, &QFileSystemWatcher::directoryChanged, this, &MimeManager1Service::onMimeAppsDirectoryChanged);
- Determine the XDG config directory (same
-
Implement the
onMimeAppsDirectoryChangedslot to re-attach the file watcher when mimeapps.list is (re)created:void MimeManager1Service::onMimeAppsDirectoryChanged(const QString &dirPath) { const QString userConfigDir = getXDGConfigHome(); if (dirPath != userConfigDir) { return; } const QString userMimeAppsFile = userConfigDir + "/mimeapps.list"; // If the file now exists but isn't being watched, re-add it. if (QFileInfo::exists(userMimeAppsFile) && !m_mimeAppsWatcher.files().contains(userMimeAppsFile)) { m_mimeAppsWatcher.addPath(userMimeAppsFile); // Optionally treat this as a "file changed" event so existing logic runs: onMimeAppsFileChanged(userMimeAppsFile); } }
-
Ensure the declaration of
onMimeAppsDirectoryChangedis added to the corresponding header (mimemanager1service.h), e.g.:private slots: void onMimeAppsFileChanged(const QString &path); void onMimeAppsDirectoryChanged(const QString &dirPath);
With these changes, the parent directory is always watched, and the file watcher is automatically reattached when mimeapps.list is deleted and later recreated.
| m_mimeAppsWatcher.addPath(userMimeAppsFile); | ||
| } | ||
|
|
||
| m_mimeAppsDebounceTimer.setSingleShot(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (complexity): Consider replacing the shared m_internalWriteInProgress flag with scoped QSignalBlocker usage and an optional helper for debounce to localize state and simplify control flow.
You can simplify the control flow and remove the fragile m_internalWriteInProgress state entirely by scoping the suppression of fileChanged using QSignalBlocker on the watcher. That keeps the “ignore internal writes” behaviour but makes it local and non‑leaky.
1. Replace m_internalWriteInProgress with a scoped signal blocker
#include <QSignalBlocker>void MimeManager1Service::setDefaultApplication(const ObjectMap &defaultApps) noexcept
{
auto &app = m_infos.front().appsList();
auto userConfig = std::find_if(app.begin(), app.end(), [](const MimeApps &config) {
return !config.isDesktopSpecific();
});
if (userConfig == app.end()) {
qWarning() << "couldn't find user mimeApps";
safe_sendErrorReply(QDBusError::InternalError);
return;
}
// Scoped suppression of QFileSystemWatcher notifications
QSignalBlocker watcherBlocker(&m_mimeAppsWatcher);
for (auto it = defaultApps.constKeyValueBegin(); it != defaultApps.constKeyValueEnd(); ++it) {
userConfig->setDefaultApplication(it->first, it->second);
}
if (!userConfig->writeToFile()) {
safe_sendErrorReply(QDBusError::Failed,
"set default app failed, these config will be reset after re-login.");
// no extra state to reset; watcherBlocker destructor re‑enables signals
return;
}
}void MimeManager1Service::unsetDefaultApplication(const QStringList &mimeTypes) noexcept
{
auto &app = m_infos.front().appsList();
auto userConfig = std::find_if(app.begin(), app.end(), [](const MimeApps &config) {
return !config.isDesktopSpecific();
});
if (userConfig == app.end()) {
qWarning() << "couldn't find user mimeApps";
safe_sendErrorReply(QDBusError::InternalError);
return;
}
QSignalBlocker watcherBlocker(&m_mimeAppsWatcher);
for (const auto &mime : mimeTypes) {
userConfig->unsetDefaultApplication(mime);
}
if (!userConfig->writeToFile()) {
safe_sendErrorReply(QDBusError::Failed,
"unset default app failed, these config will be reset after re-login.");
return;
}
}Then onMimeAppsFileChanged no longer needs to know about internal write state:
void MimeManager1Service::onMimeAppsFileChanged(const QString &path)
{
if (!m_mimeAppsWatcher.files().contains(path) && QFileInfo::exists(path)) {
m_mimeAppsWatcher.addPath(path);
}
m_mimeAppsDebounceTimer.start();
}This:
- Keeps the feature (watching and reloading
mimeapps.list). - Ensures internal writes never trigger reloads.
- Removes the cross‑method lifecycle of
m_internalWriteInProgressand its hidden coupling.
2. Optional: isolate debounce logic into a helper
To reduce the cognitive load further, you can hide the timer in a small helper and keep onMimeAppsFileChanged high‑level:
void MimeManager1Service::scheduleMimeAppsReload()
{
m_mimeAppsDebounceTimer.start();
}
void MimeManager1Service::onMimeAppsFileChanged(const QString &path)
{
if (!m_mimeAppsWatcher.files().contains(path) && QFileInfo::exists(path)) {
m_mimeAppsWatcher.addPath(path);
}
scheduleMimeAppsReload();
}This keeps all existing behaviour but makes the state management explicit, scoped, and easier to reason about.
BLumia
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
围绕 MimeInfoChanged 这个信号有点问题。
| " <arg direction=\"out\" type=\"a{oa{sa{sv}}}\" name=\"applications_and_properties\"/>\n" | ||
| " <annotation value=\"ObjectMap\" name=\"org.qtproject.QtDBus.QtTypeName.Out0\"/>\n" | ||
| " </method>\n" | ||
| " <signal name=\"MimeInfoChanged\">\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这个信号名字是不是有可能有误导,有可能文件内容实际并未实际有变化,只是因为filewatcher监视到了文件变化导致单纯的重载入了一次mime信息?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这个信号名字是不是有可能有误导,有可能文件内容实际并未实际有变化,只是因为filewatcher监视到了文件变化导致单纯的重载入了一次mime信息?
我们需要暴露出去的应该是mimeInfo变化的信号吧,监视文件变化只是我们实现的方式,如果真的出现内容未变化,而触发了信号,那也是我们后面需要修复的吧,
| qInfo() << "Reloading MIME info due to external configuration change."; | ||
| m_mimeManager->reset(); | ||
| scanMimeInfos(); | ||
| emit m_mimeManager->MimeInfoChanged(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
同其他备注,重载就一定有变化吗?以及是否把这个信号移动到 handleMimeAppsFileDebounced() 调用 reloadMimeInfos() 之后更合适?
|
|
||
| void ApplicationManager1Service::reloadMimeInfos() noexcept | ||
| { | ||
| qInfo() << "Reloading MIME info due to external configuration change."; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这个注释是不是应该写到 handleMimeAppsFileDebounced() 调用 reloadMimeInfos() 的位置?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
| // 添加用户配置目录下的 mimeapps.list 文件到监控 | ||
| QString userMimeAppsFile = getXDGConfigHome() + "/mimeapps.list"; | ||
| if (QFileInfo::exists(userMimeAppsFile)) { | ||
| m_mimeAppsWatcher.addPath(userMimeAppsFile); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
必须要存在么?这个文件会不会第一次在am起来后才创建的?
另外,如果文件不存在,要不要提示个信息,那个定时器也不用刷了吧,
|
|
||
| if (!userConfig->writeToFile()) { | ||
| safe_sendErrorReply(QDBusError::Failed, "set default app failed, these config will be reset after re-login."); | ||
| m_internalWriteInProgress = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
有定时器,是不是不需要这个m_internalWriteInProgress这个flag了,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request adds automatic MIME information reload capability when the user's mimeapps.list configuration file is modified externally. The implementation uses QFileSystemWatcher to monitor the file, implements a debounce mechanism to handle rapid changes, and includes an internal write flag to prevent reload loops from the application's own modifications.
Changes:
- Added MimeInfoChanged DBus signal to notify clients when MIME configuration is updated
- Implemented file system monitoring with debouncing (50ms) for the user's mimeapps.list file
- Added internal write protection mechanism to distinguish between internal and external file modifications
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| api/dbus/org.desktopspec.MimeManager1.xml | Added MimeInfoChanged signal definition to the DBus interface specification |
| toolGenerate/qdbusxml2cpp/org.desktopspec.MimeManager1Adaptor.h | Generated DBus adaptor header with MimeInfoChanged signal |
| src/dbus/mimemanager1service.h | Added file watcher, debounce timer, internal write flag, and signal/slot declarations |
| src/dbus/mimemanager1service.cpp | Implemented file monitoring logic, debouncing, and internal write protection |
| src/dbus/applicationmanager1service.h | Added reloadMimeInfos public method |
| src/dbus/applicationmanager1service.cpp | Implemented reloadMimeInfos to reset and rescan MIME info, emitting change signal |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| m_internalWriteInProgress = true; | ||
| for (auto it = defaultApps.constKeyValueBegin(); it != defaultApps.constKeyValueEnd(); ++it) { | ||
| userConfig->setDefaultApplication(it->first, it->second); | ||
| } | ||
|
|
||
| if (!userConfig->writeToFile()) { | ||
| safe_sendErrorReply(QDBusError::Failed, "set default app failed, these config will be reset after re-login."); | ||
| m_internalWriteInProgress = false; | ||
| return; | ||
| } |
Copilot
AI
Jan 22, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The internal write flag is not reset after writeToFile succeeds. While the design expects the file system watcher to consume this flag in onMimeAppsFileChanged, this creates a reliability issue. If the file change event doesn't fire for any reason (e.g., file system watcher malfunction, write doesn't trigger change event, or timing issues), the flag will remain true indefinitely, blocking all future external change detection. Consider resetting the flag using a timer-based fallback or RAII pattern to ensure it's always cleared, even if the file change event doesn't arrive.
| m_internalWriteInProgress = true; | ||
| for (const auto &mime : mimeTypes) { | ||
| userConfig->unsetDefaultApplication(mime); | ||
| } | ||
|
|
||
| if (!userConfig->writeToFile()) { | ||
| safe_sendErrorReply(QDBusError::Failed, "unset default app failed, these config will be reset after re-login."); | ||
| m_internalWriteInProgress = false; | ||
| return; | ||
| } |
Copilot
AI
Jan 22, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The internal write flag is not reset after writeToFile succeeds. While the design expects the file system watcher to consume this flag in onMimeAppsFileChanged, this creates a reliability issue. If the file change event doesn't fire for any reason (e.g., file system watcher malfunction, write doesn't trigger change event, or timing issues), the flag will remain true indefinitely, blocking all future external change detection. Consider resetting the flag using a timer-based fallback or RAII pattern to ensure it's always cleared, even if the file change event doesn't arrive.
| // 添加用户配置目录下的 mimeapps.list 文件到监控 | ||
| QString userMimeAppsFile = getXDGConfigHome() + "/mimeapps.list"; | ||
| if (QFileInfo::exists(userMimeAppsFile)) { | ||
| m_mimeAppsWatcher.addPath(userMimeAppsFile); | ||
| } | ||
|
|
Copilot
AI
Jan 22, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The file watcher is set up in the constructor before scanMimeInfos creates the user mimeapps.list file if it doesn't exist. If the file doesn't exist at construction time, it won't be added to the watcher. Even though there's re-add logic in onMimeAppsFileChanged at lines 181-183, it won't help because that handler is never called for unwatched files. Consider moving the file watcher setup to a separate initialization method that's called after scanMimeInfos, or add a mechanism to check and add the file to the watcher after scanMimeInfos completes.
| // 添加用户配置目录下的 mimeapps.list 文件到监控 | |
| QString userMimeAppsFile = getXDGConfigHome() + "/mimeapps.list"; | |
| if (QFileInfo::exists(userMimeAppsFile)) { | |
| m_mimeAppsWatcher.addPath(userMimeAppsFile); | |
| } | |
| // 添加用户配置目录下的 mimeapps.list 文件到监控(如果已存在) | |
| const QString userMimeAppsFile = getXDGConfigHome() + "/mimeapps.list"; | |
| if (QFileInfo::exists(userMimeAppsFile)) { | |
| m_mimeAppsWatcher.addPath(userMimeAppsFile); | |
| } | |
| // 同时监控用户配置目录本身,以便在 mimeapps.list 创建后将其加入监控 | |
| const QString userConfigDir = getXDGConfigHome(); | |
| if (!m_mimeAppsWatcher.directories().contains(userConfigDir)) { | |
| m_mimeAppsWatcher.addPath(userConfigDir); | |
| } | |
| connect(&m_mimeAppsWatcher, &QFileSystemWatcher::directoryChanged, this, | |
| [this, userConfigDir](const QString &path) { | |
| if (path != userConfigDir) { | |
| return; | |
| } | |
| const QString userMimeAppsFile = userConfigDir + "/mimeapps.list"; | |
| if (QFileInfo::exists(userMimeAppsFile) | |
| && !m_mimeAppsWatcher.files().contains(userMimeAppsFile)) { | |
| m_mimeAppsWatcher.addPath(userMimeAppsFile); | |
| } | |
| }); |
| qInfo() << "Reloading MIME info due to external configuration change."; | ||
| m_mimeManager->reset(); | ||
| scanMimeInfos(); | ||
| emit m_mimeManager->MimeInfoChanged(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
信号一般是类自己发的,给别人监听用的,
ApplicationManager1Service::reloadMimeInfos这个信号应该不需要吧,直接在handleMimeAppsFileDebounced里做了,
3d3beb0 to
5bf1ed7
Compare
|
|
||
| if (!userConfig->writeToFile()) { | ||
| safe_sendErrorReply(QDBusError::Failed, "set default app failed, these config will be reset after re-login."); | ||
| m_internalWriteInProgress = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
m_internalWriteInProgress这个是不是在写完就应该重置为false?而不是非要失败才重置为false,
|
|
||
| // 如果是内部写入导致的文件变化,忽略 | ||
| if (m_internalWriteInProgress) { | ||
| m_internalWriteInProgress = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
忽略了为什么还要重置它呀,
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 18202781743, xionglinlin The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Added file system monitoring for user mimeapps.list configuration file to detect external changes and reload MIME information automatically. The fix includes a new DBus signal to notify clients when MIME info changes, debounced file change handling, and protection against reload loops from internal writes. 1. Added MimeInfoChanged signal to DBus interface to notify clients when MIME information is updated 2. Implemented QFileSystemWatcher to monitor user's mimeapps.list file for external modifications 3. Added debounce timer (50ms) to handle rapid file change events and avoid multiple reloads 4. Added internal write flag to prevent reload loops when changes originate from the application itself 5. Created reloadMimeInfos() method to reset MIME manager and rescan all MIME information 6. Updated copyright years from 2023 to 2026 across modified files Log: Added automatic reload of MIME information when user configuration files are externally modified Influence: 1. Test that external modifications to ~/.config/mimeapps.list trigger MIME info reload 2. Verify that internal writes (setDefaultApplication/ unsetDefaultApplication) don't cause reload loops 3. Test that MimeInfoChanged signal is emitted when configuration changes 4. Verify debounce mechanism works correctly with rapid file changes 5. Test that MIME cache is properly updated after reload 6. Verify default application settings persist correctly after reload fix: 添加MIME信息变更检测和重新加载功能 添加了对用户mimeapps.list配置文件的文件系统监控,以检测外部更改并自动重 新加载MIME信息。该修复包括一个新的DBus信号用于在MIME信息变更时通知客户 端、防抖的文件变更处理以及防止内部写入导致的重载循环。 1. 在DBus接口中添加MimeInfoChanged信号,用于在MIME信息更新时通知客户端 2. 实现QFileSystemWatcher来监控用户的mimeapps.list文件的外部修改 3. 添加防抖定时器(50毫秒)以处理快速的文件变更事件,避免多次重新加载 4. 添加内部写入标志,防止应用程序自身发起更改时产生重载循环 5. 创建reloadMimeInfos()方法用于重置MIME管理器并重新扫描所有MIME信息 6. 将修改文件的版权年份从2023更新到2026 Log: 新增用户配置文件被外部修改时自动重新加载MIME信息的功能 Influence: 1. 测试外部修改~/.config/mimeapps.list文件是否会触发MIME信息重新加载 2. 验证内部写入(setDefaultApplication/unsetDefaultApplication)不会导致 重载循环 3. 测试配置变更时MimeInfoChanged信号是否正确发射 4. 验证防抖机制在快速文件变更时工作正常 5. 测试重新加载后MIME缓存是否正确更新 6. 验证默认应用程序设置在重新加载后是否正确保持 PMS: BUG-288389
deepin pr auto review这份代码实现了对 MIME 类型配置文件的监控和自动重载功能,整体设计合理,使用了防抖(debounce)机制和内部写入标志来避免重复触发和循环更新。以下是对代码的详细审查和改进建议: 1. 语法逻辑问题1:文件监控的健壮性在 改进建议: void MimeManager1Service::onMimeAppsFileChanged(const QString &path)
{
// 检查文件是否存在,如果不存在则等待文件创建
if (!QFileInfo::exists(path)) {
qWarning() << "MimeApps file removed, waiting for recreation:" << path;
return;
}
// 确保文件仍在监控列表中
if (!m_mimeAppsWatcher.files().contains(path)) {
if (!m_mimeAppsWatcher.addPath(path)) {
qWarning() << "Failed to re-add path to watcher:" << path;
}
}
// 如果是内部写入导致的文件变化,忽略
if (m_internalWriteInProgress) {
m_internalWriteInProgress = false;
return;
}
m_mimeAppsDebounceTimer.start();
}问题2:
|
|
/forcemerge |
|
This pr force merged! (status: blocked) |
Added file system monitoring for user mimeapps.list configuration file to detect external changes and reload MIME information automatically. The fix includes a new DBus signal to notify clients when MIME info changes, debounced file change handling, and protection against reload loops from internal writes.
Log: Added automatic reload of MIME information when user configuration files are externally modified
Influence:
fix: 添加MIME信息变更检测和重新加载功能
添加了对用户mimeapps.list配置文件的文件系统监控,以检测外部更改并自动重
新加载MIME信息。该修复包括一个新的DBus信号用于在MIME信息变更时通知客户
端、防抖的文件变更处理以及防止内部写入导致的重载循环。
Log: 新增用户配置文件被外部修改时自动重新加载MIME信息的功能
Influence:
PMS: BUG-288389
Summary by Sourcery
Add automatic reload of MIME information when the user MIME configuration changes and notify clients via a new DBus signal.
New Features:
Enhancements: